chore: a tiny bit safer approach#482
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #482 +/- ##
==========================================
+ Coverage 74.40% 74.42% +0.02%
==========================================
Files 110 110
Lines 10482 10468 -14
Branches 704 700 -4
==========================================
- Hits 7799 7791 -8
+ Misses 2680 2674 -6
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the safeCopy utility to use Node.js's native fs.copyFile instead of manual readFile/writeFile operations, improving efficiency and handling of concurrent operations. Additionally, it removes the aggressive cleanup step (rm) that was previously deleting the entire assets directory before copying.
Key Changes
- Replaced
readFile/writeFilewith nativecopyFileAPI for better performance and atomicity - Removed the
rmcall that was deleting the entire assets directory before copying - Added comprehensive test coverage for the
safeCopyfunction
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/generators/legacy-html/utils/safeCopy.mjs | Refactored to use copyFile instead of manual read/write operations; updated documentation |
| src/generators/legacy-html/utils/tests/safeCopy.test.mjs | Added comprehensive test suite covering various scenarios |
| src/generators/legacy-html/index.mjs | Removed aggressive rm call that deleted the entire assets directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as resolved.
This comment was marked as resolved.
|
Whoops! I thought the bot would just make a commit, not open a whole different PR. |
|
cc @aduh95 / @avivkeller / @flakey5 can y'all test if y'all see any issues with this one? I tried: Didn't find any errors. |
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
I'm no longer getting errors! Left a suggestion
|
@avivkeller PR is failing |
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@nodejs/web-infra for reviews |
|
cc @aduh95 can you test the current commit, and report back if it works fine? And if it does, approve the PR? cc @nodejs/web-infra we need more reviews too. |
|
Bump, @aduh95 :3 |
This PR removes the removal of files during also the crazy copy of files, deferring the out/ folder cleanup to
doccleanas it probably should. The original intent of thermcommand was to ensure files that do not exist on source anymore and are irrelevant will be removed, but this isn't really an issue and eventually the legacy generator will go away.Simplifed to a simple copyFile call too.